Skip to content

add xtask#66

Merged
MusicalNinjaDad merged 47 commits into
mainfrom
xtask
Apr 13, 2026
Merged

add xtask#66
MusicalNinjaDad merged 47 commits into
mainfrom
xtask

Conversation

@MusicalNinjaDad
Copy link
Copy Markdown
Owner

No description provided.

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • In xtask/src/lib.rs, Exit::from(Cmd) only surfaces stderr on non-zero exit and discards stdout; consider including stdout (or at least a truncated version) in the error message to make diagnosing failing commands easier.
  • The xtask main currently anchors all commands at Path::new("."); if this is intended to operate at the workspace root, consider resolving the workspace root explicitly (e.g., via CARGO_MANIFEST_DIR or cargo metadata) to avoid surprising behavior when run from subdirectories.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `xtask/src/lib.rs`, `Exit::from(Cmd)` only surfaces `stderr` on non-zero exit and discards `stdout`; consider including `stdout` (or at least a truncated version) in the error message to make diagnosing failing commands easier.
- The xtask `main` currently anchors all commands at `Path::new(".")`; if this is intended to operate at the workspace root, consider resolving the workspace root explicitly (e.g., via `CARGO_MANIFEST_DIR` or `cargo metadata`) to avoid surprising behavior when run from subdirectories.

## Individual Comments

### Comment 1
<location path="xtask/tests/test_fmt.rs" line_range="14-15" />
<code_context>
+    let original = fs::read_to_string("tests/fixture/src/lib.rs").unwrap();
+    let copied = fs::read_to_string(tmp.path().join("src/lib.rs")).unwrap();
+    assert_eq!(original, copied);
+    let _ = fmt(tmp.path());
+    let formatted = fs::read_to_string(tmp.path().join("src/lib.rs")).unwrap();
+    assert_ne!(original, formatted);
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Assert that `fmt` actually succeeds instead of ignoring its result, so failures in running `cargo fmt` don’t show up as confusing assertion errors.

`fmt(tmp.path())` is currently ignored, so if `cargo fmt` fails (e.g. missing rustfmt, bad Cargo.toml), the test only fails later at `assert_ne!(original, formatted)`, which is misleading.

Instead, check the command result before reading the file, e.g.:

```rust
let cmd = fmt(tmp.path());
let output = cmd.result.expect("`cargo fmt` failed to run");
assert!(
    output.status.success(),
    "`cargo fmt` exited with status {:?}\nstderr: {}",
    output.status,
    String::from_utf8_lossy(&output.stderr),
);
```

That way the test fails clearly when `cargo fmt` itself fails, and only then checks that the file changed.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread xtask/tests/test_fmt.rs Outdated
@MusicalNinjaDad MusicalNinjaDad merged commit edfce08 into main Apr 13, 2026
13 checks passed
@MusicalNinjaDad MusicalNinjaDad deleted the xtask branch April 13, 2026 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant